ChannelInitializer may be invoked multiple times when used with custo… #8620
Conversation
…m EventExecutor. Motivation: The ChannelInitializer may be invoked multipled times when used with a custom EventExecutor as removal operation may be done asynchronously. We need to guard against this. Modifications: - Change Map to Set which is more correct in terms of how we use it. - Ensure we only modify the internal Set when the handler was removed yet - Add unit test. Result: Fixes #8616.
|
@atcurtis This should fix your problem |
normanmaurer
requested review from
bryce-anderson,
ejona86,
carl-mastrangelo and
trustin
| initMap.remove(ctx); | ||
| // The removal may happen in an async fashion if the EventExecutor we use does something funky. | ||
| if (ctx.isRemoved()) { | ||
| initMap.remove(ctx); |
There was a problem hiding this comment.
What's the point of this line? Wouldn't we only get here if handlerRemoved() was called? Is the concern that someone could override handlerRemoved()?
| initMap.remove(ctx); | ||
| } else { | ||
| // Ensure we always remove from the Map in all cases to not produce a memory leak. | ||
| ctx.channel().closeFuture().addListener(new ChannelFutureListener() { |
There was a problem hiding this comment.
This will retain ctx until the channel is closed, even if handlerRemoved() is called. If this is a one-off initializer (not reused) then that could greatly increase the lifetime of some objects used during handshakes and the like.
Is this seriously the only way we can clean up properly? Is the problem that the methods can be overridden? If so, then maybe we should fix that in Netty 5? Or maybe we could avoid the check-for-double-register problem by having an alternative approach to 26aa348 in Netty 5?
It is possible to cancel the future to clean up, but the amount of effort necessary to do something simple makes me question if this is more of systemic problem that should be addressed in Netty 5.
There was a problem hiding this comment.
@ejona86 yeah I think its the best we can do for netty 4 to ensure we not leak if the user overrides handlerRemoved(...).Thats also why I check isRemoved() first.
There was a problem hiding this comment.
Perhaps we could check handlerRemoved() was overridden at instantiation time using reflection and take the fast path?
There was a problem hiding this comment.
@trustin we could ... that said I think it does not really worth it as ctx.isRemoved() should be true if the EventExecutor is not some funky custom implemention that is is used with ChannelInitializer. So I would prefer to keep things simple for now.
|
You may want to document about this behavior in Javadoc though, e.g. risks of overriding |
|
Good idea
… Am 05.12.2018 um 10:37 schrieb Trustin Lee ***@***.***>:
You may want to document about this behavior in Javadoc though, e.g. risks of overriding handlerRemoved() and using a certain EventExecutor implementation.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
@trustin actually I think it is fine to not add docs as the same is true for other methods as well. |
|
@ejona86 any other comment ? |
…m EventExecutor. (#8620) Motivation: The ChannelInitializer may be invoked multipled times when used with a custom EventExecutor as removal operation may be done asynchronously. We need to guard against this. Modifications: - Change Map to Set which is more correct in terms of how we use it. - Ensure we only modify the internal Set when the handler was removed yet - Add unit test. Result: Fixes #8616.
…m EventExecutor.
Motivation:
The ChannelInitializer may be invoked multipled times when used with a custom EventExecutor as removal operation may be done asynchronously. We need to guard against this.
Modifications:
Result:
Fixes #8616.